src: refactor DH groups to delete crypto_groups.h #43896

tniessen · 2022-07-18T18:21:34Z

Rewrite FindDiffieHellmanGroup() using OpenSSL helper functions to obtain the required constants directly, instead of loading them from our own crypto_groups.h and converting them to BIGNUMs.

This also removes the need for the struct modp_group, so we can delete crypto_groups.h altogether.

cc @nodejs/cpp-reviewers

Rewrite FindDiffieHellmanGroup() using OpenSSL helper functions to obtain the required constants directly, instead of loading them from our own crypto_groups.h and converting them to BIGNUMs. This also removes the need for the struct modp_group, so we can delete crypto_groups.h altogether.

nodejs-github-bot · 2022-07-18T18:21:38Z

Review requested:

@nodejs/crypto

nodejs-github-bot · 2022-07-19T09:59:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45548/

tniessen · 2022-07-23T12:52:37Z

@nodejs/cpp-reviewers @nodejs/crypto To aid reviews: this PR mainly changes how we obtain the large prime numbers making up standardized DH groups. Instead of defining these constants in crypto_groups.h, we can retrieve the values from OpenSSL.

An existing test guarantees that the constants remain correct:

node/test/pummel/test-crypto-dh-hash.js

Lines 40 to 59 in dffcfdd

    
           const hashes = { 
        
             modp1: '630e9acd2cc63f7e80d8507624ba60ac0757201a', 
        
             modp2: '18f7aa964484137f57bca64b21917a385b6a0b60', 
        
             modp5: 'c0a8eec0c2c8a5ec2f9c26f9661eb339a010ec61', 
        
             modp14: 'af5455606fe74cec49782bb374e4c63c9b1d132c', 
        
             modp15: '7bdd39e5cdbb9748113933e5c2623b559c534e74', 
        
             modp16: 'daea5277a7ad0116e734a8e0d2f297ef759d1161', 
        
             modp17: '3b62aaf0142c2720f0bf26a9589b0432c00eadc1', 
        
             modp18: 'a870b491bbbec9b131ae9878d07449d32e54f160', 
        
           }; 
        
           for (const name in hashes) { 
        
             const group = crypto.getDiffieHellman(name); 
        
             const private_key = group.getPrime('hex'); 
        
             const hash1 = hashes[name]; 
        
             const hash2 = crypto.createHash('sha1') 
        
                               .update(private_key.toUpperCase()).digest('hex'); 
        
             assert.strictEqual(hash1, hash2); 
        
             assert.strictEqual(group.getGenerator('hex'), '02'); 
        
           }

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: nodejs#43896

nodejs-github-bot · 2022-07-25T18:37:16Z

Landed in 3e6e908

Rewrite FindDiffieHellmanGroup() using OpenSSL helper functions to obtain the required constants directly, instead of loading them from our own crypto_groups.h and converting them to BIGNUMs. This also removes the need for the struct modp_group, so we can delete crypto_groups.h altogether. PR-URL: #43896 Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: nodejs#43896

The referenced header file does not exist anymore. Refs: #43896 PR-URL: #44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: #43896 PR-URL: #43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: #43896 PR-URL: #44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

Rewrite FindDiffieHellmanGroup() using OpenSSL helper functions to obtain the required constants directly, instead of loading them from our own crypto_groups.h and converting them to BIGNUMs. This also removes the need for the struct modp_group, so we can delete crypto_groups.h altogether. PR-URL: nodejs#43896 Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: nodejs#43896 PR-URL: nodejs#43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: nodejs#43896 PR-URL: nodejs#44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: #43896 PR-URL: #43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: #43896 PR-URL: #44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: #43896 PR-URL: #43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: #43896 PR-URL: #44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

nodejs/node#43896

* chore: update to Node.js v18 * child_process: improve argument validation nodejs/node#41305 * bootstrap: support configure-time user-land snapshot nodejs/node#42466 * chore: update GN patch * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * src: use a typed array internally for process._exiting nodejs/node#43883 * chore: lib/internal/bootstrap -> lib/internal/process * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * chore: remove redudant browserGlobals patch * chore: update BoringSSL patch * src: allow embedder-provided PageAllocator in NodePlatform nodejs/node#38362 * chore: fixup Node.js crypto tests - nodejs/node#44171 - nodejs/node#41600 * lib: add Promise methods to avoid-prototype-pollution lint rule nodejs/node#43849 * deps: update V8 to 10.1 nodejs/node#42657 * src: add kNoBrowserGlobals flag for Environment nodejs/node#40532 * chore: consolidate asar initialization patches * deps: update V8 to 10.1 nodejs/node#42657 * deps: update V8 to 9.8 nodejs/node#41610 * src,crypto: remove AllocatedBuffers from crypto_spkac nodejs/node#40752 * build: enable V8's shared read-only heap nodejs/node#42809 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 * chore: fixup ESM patch * chore: fixup patch indices * src: merge NativeModuleEnv into NativeModuleLoader nodejs/node#43824 * [API] Pass OOMDetails to OOMErrorCallback https://chromium-review.googlesource.com/c/v8/v8/+/3647827 * src: iwyu in cleanup_queue.cc * src: return Maybe from a couple of functions nodejs/node#39603 * src: clean up embedder API nodejs/node#35897 * src: refactor DH groups to delete crypto_groups.h nodejs/node#43896 * deps,src: use SIMD for normal base64 encoding nodejs/node#39775 * chore: remove deleted source file * chore: update patches * chore: remove deleted source file * lib: add fetch nodejs/node#41749 * chore: remove nonexistent node specs * test: split report OOM tests nodejs/node#44389 * src: trace fs async api nodejs/node#44057 * http: trace http request / response nodejs/node#44102 * test: split test-crypto-dh.js nodejs/node#40451 * crypto: introduce X509Certificate API nodejs/node#36804 * src: split property helpers from node::Environment nodejs/node#44056 * nodejs/node#38905 bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob * lib,src: implement WebAssembly Web API nodejs/node#42701 * fixup! deps,src: use SIMD for normal base64 encoding * fixup! src: refactor DH groups to delete crypto_groups.h * chore: fixup base64 GN file * fix: check that node::InitializeContext() returns true * chore: delete _noBrowserGlobals usage * chore: disable fetch in renderer procceses * dns: default to verbatim=true in dns.lookup() nodejs/node#39987 Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: nodejs/node#43896 PR-URL: nodejs/node#43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: nodejs/node#43896 PR-URL: nodejs/node#44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

Instead of referring users to perl to find information about supported MODP groups in crypto_groups.h, explicitly list the groups with their respective strengths and with references to the defining RFC sections. Refs: nodejs/node#43896 PR-URL: nodejs/node#43986 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

The referenced header file does not exist anymore. Refs: nodejs/node#43896 PR-URL: nodejs/node#44012 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

* chore: update to Node.js v18 * child_process: improve argument validation nodejs/node#41305 * bootstrap: support configure-time user-land snapshot nodejs/node#42466 * chore: update GN patch * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * src: use a typed array internally for process._exiting nodejs/node#43883 * chore: lib/internal/bootstrap -> lib/internal/process * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * chore: remove redudant browserGlobals patch * chore: update BoringSSL patch * src: allow embedder-provided PageAllocator in NodePlatform nodejs/node#38362 * chore: fixup Node.js crypto tests - nodejs/node#44171 - nodejs/node#41600 * lib: add Promise methods to avoid-prototype-pollution lint rule nodejs/node#43849 * deps: update V8 to 10.1 nodejs/node#42657 * src: add kNoBrowserGlobals flag for Environment nodejs/node#40532 * chore: consolidate asar initialization patches * deps: update V8 to 10.1 nodejs/node#42657 * deps: update V8 to 9.8 nodejs/node#41610 * src,crypto: remove AllocatedBuffers from crypto_spkac nodejs/node#40752 * build: enable V8's shared read-only heap nodejs/node#42809 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 * chore: fixup ESM patch * chore: fixup patch indices * src: merge NativeModuleEnv into NativeModuleLoader nodejs/node#43824 * [API] Pass OOMDetails to OOMErrorCallback https://chromium-review.googlesource.com/c/v8/v8/+/3647827 * src: iwyu in cleanup_queue.cc * src: return Maybe from a couple of functions nodejs/node#39603 * src: clean up embedder API nodejs/node#35897 * src: refactor DH groups to delete crypto_groups.h nodejs/node#43896 * deps,src: use SIMD for normal base64 encoding nodejs/node#39775 * chore: remove deleted source file * chore: update patches * chore: remove deleted source file * lib: add fetch nodejs/node#41749 * chore: remove nonexistent node specs * test: split report OOM tests nodejs/node#44389 * src: trace fs async api nodejs/node#44057 * http: trace http request / response nodejs/node#44102 * test: split test-crypto-dh.js nodejs/node#40451 * crypto: introduce X509Certificate API nodejs/node#36804 * src: split property helpers from node::Environment nodejs/node#44056 * nodejs/node#38905 bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob * lib,src: implement WebAssembly Web API nodejs/node#42701 * fixup! deps,src: use SIMD for normal base64 encoding * fixup! src: refactor DH groups to delete crypto_groups.h * chore: fixup base64 GN file * fix: check that node::InitializeContext() returns true * chore: delete _noBrowserGlobals usage * chore: disable fetch in renderer procceses * dns: default to verbatim=true in dns.lookup() nodejs/node#39987 Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

tniessen added crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 18, 2022

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 18, 2022

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2022

This comment was marked as outdated.

Sign in to view

tniessen removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 19, 2022

This was referenced Jul 20, 2022

CI Reliability 2022-07-20 nodejs/reliability#326

Open

CI Reliability 2022-07-21 nodejs/reliability#327

Open

tniessen added the review wanted PRs that need reviews. label Jul 21, 2022

This was referenced Jul 22, 2022

CI Reliability 2022-07-22 nodejs/reliability#328

Open

CI Reliability 2022-07-23 nodejs/reliability#329

Open

panva approved these changes Jul 23, 2022

View reviewed changes

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 23, 2022

github-actions bot mentioned this pull request Jul 24, 2022

CI Reliability 2022-07-24 nodejs/reliability#330

Open

17 tasks

tniessen mentioned this pull request Jul 25, 2022

doc: list supported MODP groups explicitly #43986

Merged

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2022

nodejs-github-bot merged commit 3e6e908 into nodejs:main Jul 25, 2022

danielleadams mentioned this pull request Jul 26, 2022

v18.7.0 proposal #43993

Merged

tniessen added a commit to tniessen/node that referenced this pull request Jul 27, 2022

doc: remove old reference from crypto/README.md

2ba0b60

The referenced header file does not exist anymore. Refs: nodejs#43896

tniessen mentioned this pull request Jul 27, 2022

doc: remove old reference from crypto/README.md #44012

Closed

targos added the dont-land-on-v16.x label Sep 5, 2022

gengjiawen mentioned this pull request Sep 12, 2022

build: remove redundant entry in crypto #44604

Merged

codebytere added a commit to electron/electron that referenced this pull request Oct 13, 2022

src: refactor DH groups to delete crypto_groups.h

1845867

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 13, 2022

src: refactor DH groups to delete crypto_groups.h

b83cb0c

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 13, 2022

src: refactor DH groups to delete crypto_groups.h

a010057

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 13, 2022

src: refactor DH groups to delete crypto_groups.h

dfe7c4f

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 17, 2022

src: refactor DH groups to delete crypto_groups.h

2407320

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022

src: refactor DH groups to delete crypto_groups.h

4909fd0

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022

src: refactor DH groups to delete crypto_groups.h

3d3d0f7

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Oct 24, 2022

src: refactor DH groups to delete crypto_groups.h

388ef09

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022

src: refactor DH groups to delete crypto_groups.h

4732be9

nodejs/node#43896

codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022

src: refactor DH groups to delete crypto_groups.h

4ea70ea

nodejs/node#43896

Clem-Fern mentioned this pull request Aug 18, 2023

SSH: unable to connect to ssh server using kex algorithm diffie-hellman-group1-sha1 Eugeny/tabby#8840

Closed

Eugeny mentioned this pull request Aug 21, 2023

[Feature Request]: Re-add support for modp1 and modp2 Diffie-Hellman groups following Node update electron/electron#39603

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: refactor DH groups to delete crypto_groups.h #43896

src: refactor DH groups to delete crypto_groups.h #43896

tniessen commented Jul 18, 2022

nodejs-github-bot commented Jul 18, 2022

This comment was marked as outdated.

nodejs-github-bot commented Jul 19, 2022

tniessen commented Jul 23, 2022

nodejs-github-bot commented Jul 25, 2022

src: refactor DH groups to delete crypto_groups.h #43896

src: refactor DH groups to delete crypto_groups.h #43896

Conversation

tniessen commented Jul 18, 2022

nodejs-github-bot commented Jul 18, 2022

This comment was marked as outdated.

nodejs-github-bot commented Jul 19, 2022

tniessen commented Jul 23, 2022

nodejs-github-bot commented Jul 25, 2022